Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Authorization Callback Function #108

Merged
merged 11 commits into from
Oct 21, 2024
Merged

Conversation

henryluo
Copy link
Contributor

@henryluo henryluo commented Oct 17, 2024

Description

This feature introduces a callback function that allows service owners to define a fallback mechanism for access control within Knox. This capability supports the integration of custom logic to evaluate access decisions.

Changes Made

  • Add new global accessCallback and SetAccessCallback that allows setting a callback function for fallback Access (ex. OPA)
  • Add authorizeRequest function to wrap around principal.CanAccess, which additionally will execute the accessCallback when false is returned from principal.CanAccess
  • Add a Raw() to all Principal types, which raw version (jsonable ID and Type) of all the principals.
  • Modify TestNewFileClient to skip test if running on Linux and have the Knox daemon running

Testing

Run all tests in the repo, including new ones implemented for SetAccessCallback and authorizeRequest

Checklist

  • I have run the code locally and it works as expected.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate).
  • I have reviewed my own code and ensured it follows the code style of this project.

server/api_test.go Outdated Show resolved Hide resolved
server/routes_test.go Outdated Show resolved Hide resolved
server/routes_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jimmyl02 jimmyl02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall this approach looks fine to me; I'm curious if we considered adding the authorize fallback function as a part of the PrincipalMux struct. That way we could keep the same CanAccess pattern and just modify the principalMux to fallback to this authorizer. Another benefit I see is that we don't have a global variable holding the fallback authorization function and instead it's bound to a struct which should be safer in terms of different pieces of code accidentally modifying the function

@krockpot krockpot merged commit 62dd952 into pinterest:master Oct 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants